Update rs_launch.py#2764
Conversation
eb7643e to
4c53382
Compare
|
I've edited a commit. command topic list command topic list |
|
I also recommend adding this parameter to the Node parameters, so when run "ros2 param list" you will see this parameter along with all the nodes parameter, like camera_name. |
SamerKhshiboun
left a comment
There was a problem hiding this comment.
Left some comments, please fix and update the PR
|
|
||
|
|
||
| configurable_parameters = [{'name': 'camera_name', 'default': 'camera', 'description': 'camera unique name'}, | ||
| {'name': 'namespace', 'default': '', 'description': 'namespace for camera'}, |
There was a problem hiding this comment.
I don't want to surprise current users if they pull from this branch and build again.
Most of them used to see /camera/camera when launching our node, so I would prefer to set the default value in the namespace to "camera", to keep this behavior.
There was a problem hiding this comment.
Also, I think it is better to name this parameter camera_namespace to make it more clean for users that it defines a namespace for the camera.
There was a problem hiding this comment.
sure let's go with camera_namespace
4c53382 to
6b02bb3
Compare
6b02bb3 to
114350d
Compare
|
@SamerKhshiboun |
114350d to
c6faad4
Compare
|
So final one is right one I think
`namespace does not suffix. The format what I expected shall be this. |
Hi @hyunseok-yang ,
So, could you update the |
Like you mentioned, in case of multiple cameras are connected, multiple camera could be separated by only I understood intention of As I told above, what I expected on topic list is like below. However if the suffix attached after namespace How can I separate the multi-robot scenaro to distinguish cameras for each robot? You mentioned that _param_name_suffix Could you explain more specific scenario? |
|
1- We agree we need our suffix to distinguish between two nodes parameters when using rs_multi_camera_launch.py You will get this when listing the nodes in anther terminal: And this is wrong. The solution for this is to do the following: And ROS will append the namespace to the camera name, then we will get: |
|
Hi @hyunseok-yang, Do you want both camera nodes to be under the same namespace ? |
That's correct. When I command this. os2 launch realsense2_camera rs_launch.py align_depth.enable:=true rgb_camera.profile:=640x480x6 depth_module.profile:=640x480x6 camera_namespace:=test camera_name:=cam1I got this.
If I do this, only I got these topics without camera name. |
No problem :)
Yes It's why I insisting this launch script. |
do you run can you please verify your changes with |
Sure, after colcon build and setup.bash, I just run below command.
and I got this this branch |
|
Oh Now I got it. You did In case of me, I got this. It's a /namespace/camera_name/camera_name But this is what this package expected since namespace is "namespace/camera_name" |
|
If you get a node that is all topics should be the node name and then a string can you please run |
Sure, |
|
What did you get if run |
|
Just in case, I removed binary package And still got same result. |
Hi @hyunseok-yang , I need to clarify a bit more about The idea is to run the |
OK, Let me know if it is clarified to you.
My idea is like this. What do you think? |
|
To conclude everything: A namespace can include nodes, topics, etc.. Today, when launching with: We get this output, and we want to keep like this, because many users have assumption on node names and topic names The first /camera is always the namespace so the node name is and this should be like this always. the topics are What I suggest is here: in rs_launch.py edit namespace in both places: in rs_multi_camera_launch.py, edit local_parameters to be: Then, you can run this example from your terminal: ( I have D455 and D435i cameras) and then you will get two nodes (ros2 node list): and you will get these topics: If that good for you, please do the suggested changes and commit, and I will approve this PR and merge it. |
|
As you mentioned this will NOT include camera name on topics. Is that your intention? So whatever camera_name is passed on launcher, it will not affect topic name right? It it so, we have to append a camera_name on for examples, I think it is little weired that the camera |
I understand that this is weird, but your proposal is doing the same weirdness under the hood (in the code itself) when you append the Anyway, I think the best solution should be enhancing other places in the code, where we define the topic names. What do you think ? |
I didn't intend it. I appended it because code are written in this packges.
If you can edit the code as what we desired, I would be happy :). So you mean, I will get below result, isn't it? with this launch command THEN I will follow your way. After all of modifictaion, is it still required |
Lets start with your PR, and then I will continue in a separated PR (to add the camera name to the topic name) Please fix two things:: in in |
OK. just moment |
c6faad4 to
2ad2519
Compare
|
Please check, I modified the commit as what you require. |
2ad2519 to
37cb7da
Compare
- Add namespace launch argument
37cb7da to
16d49e0
Compare
|
@SamerKhshiboun Please check with latest commit 16d49e0 |

add namespace launch argument